massgen/frontend/displays/textual_widgets/setup_wizard.py-469-494 (1)

469-494: ⚠️ Potential issue | 🟡 Minor

Potential pipe deadlock: read stderr only after stdout is fully consumed.

Both stdout and stderr use PIPE. The code reads stdout line-by-line to completion before calling proc.stderr.read(). If docker pull writes enough to stderr to fill the OS pipe buffer (~64 KB) before closing stdout, the subprocess will block on the stderr write and never close stdout, creating a deadlock.

For docker pull this is unlikely in practice (stderr is small), but consider using proc.communicate() or reading both streams concurrently for robustness.

Suggested safer approach

-                proc = await asyncio.create_subprocess_exec(
-                    "docker",
-                    "pull",
-                    image,
-                    stdout=asyncio.subprocess.PIPE,
-                    stderr=asyncio.subprocess.PIPE,
-                )
-                # Stream stdout line-by-line to show layer progress
-                if proc.stdout:
-                    async for raw_line in proc.stdout:
-                        line = raw_line.decode(errors="replace").strip()
-                        if line:
-                            self._spinner_message = line
-                stderr = await proc.stderr.read() if proc.stderr else b""
-                await proc.wait()
+                proc = await asyncio.create_subprocess_exec(
+                    "docker",
+                    "pull",
+                    image,
+                    stdout=asyncio.subprocess.PIPE,
+                    stderr=asyncio.subprocess.PIPE,
+                )
+
+                stderr_chunks: list[bytes] = []
+
+                async def _drain_stderr():
+                    if proc.stderr:
+                        async for chunk in proc.stderr:
+                            stderr_chunks.append(chunk)
+
+                stderr_task = asyncio.create_task(_drain_stderr())
+
+                if proc.stdout:
+                    async for raw_line in proc.stdout:
+                        line = raw_line.decode(errors="replace").strip()
+                        if line:
+                            self._spinner_message = line
+
+                await stderr_task
+                stderr = b"".join(stderr_chunks)
+                await proc.wait()

massgen/frontend/displays/textual_widgets/file_explorer_panel.py-44-48 (1)

44-48: ⚠️ Potential issue | 🟡 Minor

Placeholder entries are unintentionally clickable — clicking shows "file not found".

absolute_path="" is falsy, so _add_path (line 57: absolute_path or display_path) stores the display text "... (N more files)" as the tree-node data. That string is truthy, so on_tree_node_selected calls _show_preview, which fails to find the "file" and shows a confusing "file not found" message. The same issue applies to line 141.

Fix _add_path to distinguish an explicit empty string from None:

Proposed fix

 def _add_path(self, display_path: str, status: str, absolute_path: Optional[str] = None) -> None:
     """Track a path for display and lookup."""
     self._all_paths[display_path] = status
-    self._path_lookup[display_path] = absolute_path or display_path
+    self._path_lookup[display_path] = absolute_path if absolute_path is not None else display_path

massgen/configs/debug/test_write_mode_isolation.yaml-1-38 (1)

1-38: ⚠️ Potential issue | 🟡 Minor

Config placement doesn’t follow required configs/ subdirectory structure.

Please move this file under one of the approved subdirectories (basic/, tools/, providers/, teams/) to keep the configs tree consistent.

Based on learnings “Configuration YAML files in massgen/configs/ should be organized into subdirectories: basic/ for simple single/multi-agent configs, tools/ for MCP/filesystem/code execution configs, providers/ for provider-specific examples, and teams/ for pre-configured specialized teams”.

massgen/infrastructure/shadow_repo.py-69-72 (1)

69-72: ⚠️ Potential issue | 🟡 Minor

Chain the original exception for debuggability.

raise RuntimeError(...) discards the original GitCommandError traceback. Use from e to preserve the exception chain. Also, logging.exception would auto-include the traceback.

Proposed fix

         except GitCommandError as e:
-            logger.error(f"Failed to initialize shadow repo: {e.stderr}")
+            logger.exception(f"Failed to initialize shadow repo: {e.stderr}")
             self.cleanup()
-            raise RuntimeError(f"Failed to initialize shadow repo: {e.stderr}")
+            raise RuntimeError(f"Failed to initialize shadow repo: {e.stderr}") from e

massgen/infrastructure/shadow_repo.py-127-139 (1)

127-139: ⚠️ Potential issue | 🟡 Minor

Docstring claims .gitignore is respected, but only .git dirs are skipped.

The docstring says "respecting .gitignore if present" but the implementation only filters out .git directories via shutil.ignore_patterns(".git"). Files that would be git-ignored (e.g., __pycache__, node_modules) are still copied. Either update the docstring to reflect actual behavior, or add .gitignore parsing.

Proposed docstring fix

     def _copy_source_files(self) -> None:
-        """Copy files from source to temp_dir, respecting .gitignore if present."""
+        """Copy files from source to temp_dir, excluding .git directories."""

massgen/utils/git_utils.py-109-134 (1)

109-134: ⚠️ Potential issue | 🟡 Minor

get_changes docstring claims staged changes are included, but they aren't.

The docstring says "Get list of all changes (staged, unstaged, untracked)" but the implementation only collects unstaged changes (repo.index.diff(None)) and untracked files. Staged changes (repo.index.diff("HEAD")) are not captured. Either update the docstring to say "unstaged and untracked" or add the staged diff.

Option A: Fix docstring to match implementation

 def get_changes(repo: Repo) -> List[Dict[str, str]]:
     """
-    Get list of all changes (staged, unstaged, untracked) in a repo.
+    Get list of unstaged and untracked changes in a repo.

Option B: Add staged changes to match docstring

     changes = []
 
+    # Staged changes (index vs HEAD)
+    try:
+        for diff in repo.index.diff("HEAD"):
+            changes.append(
+                {
+                    "status": diff.change_type[0].upper(),
+                    "path": diff.a_path or diff.b_path,
+                },
+            )
+    except Exception:
+        pass  # Empty repo with no HEAD commit
+
     # Unstaged changes (working tree vs index)
     for diff in repo.index.diff(None):

massgen/cli.py-1471-1474 (1)

1471-1474: ⚠️ Potential issue | 🟡 Minor

Preserve explicit falsy write_mode values.

if write_mode_setting: skips explicit falsy values (e.g., false in YAML). If False is a valid override, it won’t propagate to the backend config.

🔧 Proposed fix

-        write_mode_setting = coordination_settings_for_injection.get("write_mode")
-        if write_mode_setting:
+        write_mode_setting = coordination_settings_for_injection.get("write_mode")
+        if write_mode_setting is not None:
             backend_config["write_mode"] = write_mode_setting

massgen/tests/test_isolation_context.py-725-731 (1)

725-731: ⚠️ Potential issue | 🟡 Minor

Remove unused tmp_path fixture to satisfy lint.

Proposed fix

-    def test_remove_nonexistent_path_returns_none(self, tmp_path):
+    def test_remove_nonexistent_path_returns_none(self):

massgen/tests/test_isolation_context.py-17-30 (1)

17-30: ⚠️ Potential issue | 🟡 Minor

Add Google-style Args/Returns to init_test_repo docstring.

Proposed fix

 def init_test_repo(path: Path, with_commit: bool = True) -> Repo:
-    """Helper to initialize a test git repo with GitPython."""
+    """Helper to initialize a test git repo with GitPython.
+
+    Args:
+        path: Directory to initialize as a git repository.
+        with_commit: Whether to create an initial commit.
+
+    Returns:
+        Initialized GitPython Repo instance.
+    """

As per coding guidelines: `**/*.py`: For new or changed functions, include Google-style docstrings.

massgen/frontend/displays/textual/widgets/modals/review_modal.py-35-67 (1)

35-67: ⚠️ Potential issue | 🟡 Minor

Add Google-style docstrings to _ApprovalToggle / _FileEntry methods.

The new __init__ and on_click handlers are missing Google-style docstrings.

As per coding guidelines: **/*.py: For new or changed functions, include Google-style docstrings.

scripts/test_review_modal.py-183-223 (1)

183-223: ⚠️ Potential issue | 🟡 Minor

Add Google-style Args/Returns to _build_combined_css docstring.

Proposed fix

 def _build_combined_css(theme: str = "dark") -> Path:
-    """Build a combined CSS file from palette + modal_base + base, matching the real app.
-
-    Always writes to the same fixed path so the stylesheet can be re-read on theme toggle.
-    """
+    """Build a combined CSS file from palette + modal_base + base, matching the real app.
+
+    Always writes to the same fixed path so the stylesheet can be re-read on theme toggle.
+
+    Args:
+        theme: Theme name ("dark" or "light").
+
+    Returns:
+        Path to the combined CSS file on disk.
+    """

As per coding guidelines: `**/*.py`: For new or changed functions, include Google-style docstrings.

scripts/test_review_modal.py-25-31 (1)

25-31: ⚠️ Potential issue | 🟡 Minor

Remove unused noqa: E402 directives to avoid Ruff warnings.

Proposed fix

-from textual.app import App, ComposeResult  # noqa: E402
-from textual.containers import Center, Vertical  # noqa: E402
-from textual.widgets import Button, Footer, Header, Label, Static  # noqa: E402
+from textual.app import App, ComposeResult
+from textual.containers import Center, Vertical
+from textual.widgets import Button, Footer, Header, Label, Static
 
-from massgen.filesystem_manager import ReviewResult  # noqa: E402
+from massgen.filesystem_manager import ReviewResult
 from massgen.frontend.displays.textual.widgets.modals.review_modal import (  # noqa: E402
     GitDiffReviewModal,
 )

scripts/test_review_modal.py-231-333 (1)

231-333: ⚠️ Potential issue | 🟡 Minor

Add Google-style docstrings to ReviewModalTestApp public methods.

compose, on_button_pressed, action_*, _open_modal, and _handle_result are new and should include Google-style docstrings (summary + Args/Returns where relevant).

As per coding guidelines: **/*.py: For new or changed functions, include Google-style docstrings.

massgen/filesystem_manager/_change_applier.py-144-191 (1)

144-191: ⚠️ Potential issue | 🟡 Minor

_apply_file_changes does not handle file deletions.

The fallback method walks the source to find new/modified files, but never detects files that exist in the target but were deleted in the source. This means deletions would silently be ignored when the git-based path fails. Consider walking the target as well to detect removed files.

massgen/infrastructure/worktree_manager.py-25-29 (1)

25-29: ⚠️ Potential issue | 🟡 Minor

repo.working_tree_dir can be None for bare repositories.

If a bare repo path is passed, Repo() succeeds but working_tree_dir returns None, leaving self.repo_path = None silently. Subsequent operations would fail with confusing errors.

Proposed fix

         try:
             self.repo = Repo(repo_path, search_parent_directories=True)
             self.repo_path = self.repo.working_tree_dir
+            if self.repo_path is None:
+                raise ValueError(f"Path {repo_path} is in a bare git repository (no working tree)")
         except InvalidGitRepositoryError:
-            raise ValueError(f"Path {repo_path} is not in a git repository")
+            raise ValueError(f"Path {repo_path} is not in a git repository") from None

massgen/filesystem_manager/_filesystem_manager.py-411-429 (1)

411-429: ⚠️ Potential issue | 🟡 Minor

Non-git context paths are silently dropped when write_mode is active.

When write_mode is set, Line 426 clears context_paths = [] unconditionally. However, extra_mount_paths only includes entries for context paths that have a .git directory (Line 420). If a user configures a context path that is not a git repository, it will neither be mounted as a regular context path nor as an extra .git mount — effectively making it invisible to the agent with no warning.

Consider logging a warning (or raising a validation error upstream) for context paths that lack a .git directory when write_mode requires worktree-based isolation.

Proposed fix

                 if ctx_path and os.path.isdir(git_dir):
                     extra_mount_paths.append((git_dir, git_dir, "rw"))
                     logger.info(
                         f"[FilesystemManager] write_mode: mounting .git/ dir for worktree refs: {git_dir}",
                     )
+                elif ctx_path:
+                    logger.warning(
+                        f"[FilesystemManager] write_mode: context path {ctx_path} has no .git directory — "
+                        "it will not be mounted. Worktree isolation requires a git repository.",
+                    )

massgen/frontend/displays/textual_themes/base.tcss-4966-4976 (1)

4966-4976: ⚠️ Potential issue | 🟡 Minor

SubagentScreen AgentTabBar needs .theme-light/.theme-dark overrides for consistency.

AgentStatusRibbon and #input_header both pair $separator-border with explicit .theme-light/.theme-dark hex overrides (lines ~396–403, ~1093–1100). SubagentScreen AgentTabBar (line 4969) lacks equivalent overrides. While AgentStatusRibbon inherits theme rules via global descendant selectors, AgentTabBar has no corresponding theme-specific rules anywhere, risking incorrect border visibility in light mode.

massgen/frontend/displays/textual_terminal_display.py-2907-2977 (1)

2907-2977: ⚠️ Potential issue | 🟡 Minor

Avoid writing debug JSON to a fixed /tmp path.

Even behind a debug flag, fixed /tmp/textual_debug.json can be clobbered or redirected (symlink attacks). Use a secure temp file or the session log directory to avoid predictable paths.

🛠️ Suggested fix

-                with open("/tmp/textual_debug.json", "w") as f:
-                    json.dump(debug_info, f, indent=2, default=str)
-                self.log("DEBUG: Widget info written to /tmp/textual_debug.json")
+                import tempfile
+                with tempfile.NamedTemporaryFile("w", suffix=".json", delete=False) as f:
+                    json.dump(debug_info, f, indent=2, default=str)
+                    debug_path = f.name
+                self.log(f"DEBUG: Widget info written to {debug_path}")

massgen/frontend/displays/textual_terminal_display.py-1687-1721 (1)

1687-1721: ⚠️ Potential issue | 🟡 Minor

Timeout handler can pop the wrong screen.

On timeout, pop_screen() is invoked without checking that the review modal is still the active screen. If another modal was opened (or the review modal already closed), this can dismiss the wrong UI. Track the modal instance and only pop when it’s still on top (similar to the broadcast modal flow).

🛠️ Suggested fix

-        loop = asyncio.get_running_loop()
-        result_future: asyncio.Future[ReviewResult] = loop.create_future()
+        loop = asyncio.get_running_loop()
+        result_future: asyncio.Future[ReviewResult] = loop.create_future()
+        modal_ref = {"modal": None}
@@
-                modal = GitDiffReviewModal(changes=changes)
+                modal = GitDiffReviewModal(changes=changes)
+                modal_ref["modal"] = modal
@@
-            # Try to dismiss the modal on timeout
-            try:
-                self._app.call_from_thread(self._app.pop_screen)
-            except Exception:
-                pass
+            # Try to dismiss the modal on timeout (only if it’s still on top)
+            def safe_pop():
+                if self._app and modal_ref["modal"] and self._app.screen_stack:
+                    if self._app.screen_stack[-1] is modal_ref["modal"]:
+                        self._app.pop_screen()
+            try:
+                self._app.call_from_thread(safe_pop)
+            except Exception:
+                pass

massgen/frontend/displays/textual_terminal_display.py-603-607 (1)

603-607: ⚠️ Potential issue | 🟡 Minor

Final-presentation gating still allows default “thinking” content to leak into the timeline.

The comment says only tools should pass, but the allowlist includes "thinking". Since content_type defaults to "thinking", final-presentation chunks can still append to the timeline and reintroduce duplicates. If the goal is to suppress all non-tool content in this phase, drop "thinking" from the allowlist or ensure final presentation uses content_type="presentation".

🛠️ Possible adjustment (if tools-only is intended)

-        if hasattr(self, "_in_final_presentation") and self._in_final_presentation:
-            if content_type not in ("tool", "thinking"):
-                return
+        if hasattr(self, "_in_final_presentation") and self._in_final_presentation:
+            if content_type != "tool":
+                return

massgen/tests/test_write_mode_scratch.py-142-142 (1)

142-142: ⚠️ Potential issue | 🟡 Minor

Typo in class names: "Scatch" → "Scratch".

TestMoveScatchToWorkspace (line 142) and TestShadowModeCreatesScatch (line 405) both misspell "Scratch."

Fix

-class TestMoveScatchToWorkspace:
+class TestMoveScratchToWorkspace:

-class TestShadowModeCreatesScatch:
+class TestShadowModeCreatesScratch:

Also applies to: 405-405

massgen/system_prompt_sections.py-949-971 (1)

949-971: ⚠️ Potential issue | 🟡 Minor

Branch info and scratch-archive note are duplicated once per worktree path.

The for wt_path in self.worktree_paths: loop (line 951) contains the "### Code Branches" header, branch_name, other_branches, and the .scratch_archive/ note. When multiple context paths produce multiple worktree entries, all of this gets repeated for every iteration. Only the "### Project Checkout" block is per-worktree; the branch/coordination info should be rendered once, outside the loop.

Proposed fix: move branch info outside the loop

         # Worktree-based workspace (new unified model) takes precedence
         if self.worktree_paths:
             for wt_path in self.worktree_paths:
                 content_parts.append("### Project Checkout\n")
                 content_parts.append(f"The project is checked out at `{wt_path}`. Full read/write access.\n")
                 content_parts.append("**Scratch Space**: `.massgen_scratch/` inside the checkout")
                 content_parts.append("- For experiments, eval scripts, notes")
                 content_parts.append("- Git-excluded, invisible to reviewers")
                 content_parts.append("- Can import from project naturally (e.g., `from src.foo import bar`)\n")
 
-                content_parts.append("### Code Branches\n")
-                if self.branch_name:
-                    content_parts.append(f"Your work is on branch `{self.branch_name}`. All changes are auto-committed when your turn ends.\n")
-                else:
-                    content_parts.append("All changes are auto-committed when your turn ends.\n")
-
-                if self.other_branches:
-                    content_parts.append("**Other agents' branches:**")
-                    for label, branch in self.other_branches.items():
-                        content_parts.append(f"- {label}: `{branch}`")
-                    content_parts.append("\nUse `git diff <branch>` to compare, `git merge <branch>` to incorporate.\n")
-
-                content_parts.append("**Previous scratch files**: Check `.scratch_archive/` in your workspace for experiments from prior rounds.\n")
+            content_parts.append("### Code Branches\n")
+            if self.branch_name:
+                content_parts.append(f"Your work is on branch `{self.branch_name}`. All changes are auto-committed when your turn ends.\n")
+            else:
+                content_parts.append("All changes are auto-committed when your turn ends.\n")
+
+            if self.other_branches:
+                content_parts.append("**Other agents' branches:**")
+                for label, branch in self.other_branches.items():
+                    content_parts.append(f"- {label}: `{branch}`")
+                content_parts.append("\nUse `git diff <branch>` to compare, `git merge <branch>` to incorporate.\n")
+
+            content_parts.append("**Previous scratch files**: Check `.scratch_archive/` in your workspace for experiments from prior rounds.\n")

massgen/orchestrator.py-8659-8675 (1)

8659-8675: ⚠️ Potential issue | 🟡 Minor

Unused parameter in _review_isolated_changes.

agent isn’t used; either remove it or prefix with _ to satisfy lint and clarify intent.

🔧 Suggested fix

-    async def _review_isolated_changes(
-        self,
-        agent: "ChatAgent",
+    async def _review_isolated_changes(
+        self,
+        _agent: "ChatAgent",
         isolation_manager: "IsolationContextManager",
         selected_agent_id: str,
     ) -> AsyncGenerator[StreamChunk, None]:

massgen/orchestrator.py-7893-7905 (1)

7893-7905: ⚠️ Potential issue | 🟡 Minor

Avoid silent isolation cleanup failures.

The try/except: pass masks cleanup issues and will make isolation failures hard to debug. Logging at debug is safer and satisfies lint.

🧹 Suggested fix

-                    if self._isolation_manager:
-                        try:
-                            self._isolation_manager.cleanup_all()
-                        except Exception:
-                            pass
+                    if self._isolation_manager:
+                        try:
+                            self._isolation_manager.cleanup_all()
+                        except Exception as cleanup_err:
+                            logger.debug(
+                                f"[Orchestrator] Isolation cleanup failed during fallback: {cleanup_err}",
+                            )

massgen/orchestrator.py-5828-5883 (1)

5828-5883: ⚠️ Potential issue | 🟡 Minor

Clean up partial worktrees on setup failure.

If setup fails mid‑way, lingering worktrees/branches can leak and affect later rounds. Consider cleaning up the partially created isolation session in the exception path.

🔧 Suggested fix

-        if write_mode and write_mode != "legacy" and agent.backend.filesystem_manager:
-            try:
+        if write_mode and write_mode != "legacy" and agent.backend.filesystem_manager:
+            round_isolation_mgr = None
+            try:
                 from .filesystem_manager import IsolationContextManager
@@
-                round_isolation_mgr = IsolationContextManager(
+                round_isolation_mgr = IsolationContextManager(
                     session_id=f"{self.session_id}-{round_suffix}",
                     write_mode=write_mode,
                     workspace_path=workspace_path,
                     previous_branch=previous_branch,
                 )
@@
-            except Exception as e:
-                logger.warning(f"[Orchestrator] Failed to create per-round worktree for {agent_id}: {e}")
-                round_worktree_paths = None
+            except Exception as e:
+                if round_isolation_mgr:
+                    try:
+                        round_isolation_mgr.cleanup_session()
+                    except Exception as cleanup_err:
+                        logger.debug(
+                            f"[Orchestrator] Failed to cleanup partial worktree for {agent_id}: {cleanup_err}",
+                        )
+                logger.warning(f"[Orchestrator] Failed to create per-round worktree for {agent_id}: {e}")
+                round_worktree_paths = None

massgen/filesystem_manager/_isolation_context_manager.py-506-510 (1)

506-510: ⚠️ Potential issue | 🟡 Minor

shutil.move behaves differently when destination already exists.

If move_scratch_to_workspace is called twice with the same archive_label, the second call will nest the scratch directory inside the existing archive directory (e.g., .scratch_archive/agent1/.massgen_scratch/) instead of replacing it. This could surprise consumers of the archive.

Proposed fix: remove existing archive dir before moving

         archive_dir = os.path.join(workspace, ".scratch_archive", archive_name)
         os.makedirs(os.path.dirname(archive_dir), exist_ok=True)
 
         try:
+            if os.path.exists(archive_dir):
+                shutil.rmtree(archive_dir)
             shutil.move(scratch_path, archive_dir)

massgen/filesystem_manager/_isolation_context_manager.py-214-220 (1)

214-220: ⚠️ Potential issue | 🟡 Minor

Branch-name collision is possible when branch_label is set and multiple context paths exist in the same repository, though this is an edge case.

When initialize_context is called multiple times with different context_path values that point to the same git repository (e.g., repo/src and repo/tests both within repo), and branch_label is provided (e.g., "presenter"), the first call succeeds but the second will fail. The initialize_context cache is keyed by individual context_path, not by repo_root, so it cannot prevent the collision. When git worktree add -b "presenter" is called the second time, it will raise GitCommandError because the branch already exists, which propagates as a RuntimeError.

While multiple writable context paths in the same repository during final presentation is not a typical usage pattern and lacks test coverage, it is technically possible through the orchestrator's managed paths system. Consider either: (1) detecting this scenario and reusing the existing branch/worktree for same-repo paths, (2) appending a suffix to branch_label per context path, or (3) documenting that branch_label assumes a single context path per repository during final presentation.

massgen/frontend/displays/textual_widgets/multi_line_input.py (1)

601-608: Pre-existing: redundant identical branches in _delete_to_position.

Both branches of the ternary on Line 603 evaluate to the same expression (target_col + 1), and similarly Line 607 evaluates to target_col in both branches. The logic happens to be correct because _find_char_position already adjusts target_col for t/T vs f/F, but the dead conditionals are confusing and suggest the author may have intended different behavior per motion type.

Not introduced by this PR, so just flagging for awareness.

♻️ Suggested simplification

         if motion in ("f", "t"):
             # Delete forward (include target for 'f', exclude for 't' already handled)
-            end = target_col + 1 if motion == "f" else target_col + 1
+            end = target_col + 1
             lines[row] = line[:col] + line[end:]
         else:
             # Delete backward
-            start = target_col if motion == "F" else target_col
+            start = target_col
             lines[row] = line[:start] + line[col:]
             col = start  # Move cursor to deletion point

massgen/frontend/displays/textual_widgets/setup_wizard.py (3)

519-521: Successfully-pulled image checkboxes are re-enabled and remain interactive.

After a successful pull, _selected_images is trimmed (line 503) but the corresponding checkboxes are re-enabled on line 520–521. A user could re-check a successfully-pulled image, causing it to be re-added to _selected_images via on_checkbox_changed and re-pulled on retry.

Consider disabling or removing checkboxes for successfully-pulled images.

Suggested fix

         # Re-enable remaining checkboxes
-        for cb in self._checkboxes.values():
-            cb.disabled = False
+        for img_name, cb in self._checkboxes.items():
+            if img_name in pulled:
+                cb.value = False
+                cb.disabled = True
+            else:
+                cb.disabled = False

---

304-310: Instance attribute _SPINNER_FRAMES uses UPPER_CASE naming.

PEP 8 reserves UPPER_CASE for class-level constants. Since this is an instance attribute, either promote it to a class constant (like AVAILABLE_IMAGES) or rename it to _spinner_frames.

Suggested fix

Move to class level alongside AVAILABLE_IMAGES:

 class DockerSetupStep(StepComponent):
     ...
     AVAILABLE_IMAGES = [...]
+    _SPINNER_FRAMES = ("⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏")

     def __init__(self, ...):
         ...
-        self._SPINNER_FRAMES = ("⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏")

This aligns with the pattern in textual_terminal_display.py where SPINNER_FRAMES is a class-level attribute (see relevant code snippet at line 2339).

---

1016-1025: Fragile .env parsing doesn't handle quoted values or inline comments.

The manual .env parser (lines 1020–1025) splits on = and strips whitespace, but doesn't handle quoted values (KEY="value") or inline comments (KEY=value # comment). If any existing .env entry uses these patterns, the re-written file will corrupt them.

Consider using dotenv's own dotenv_values() to read the existing file, since python-dotenv is already a dependency.

Suggested fix

+        from dotenv import dotenv_values
+
         existing_content = {}
         if env_path.exists():
-            try:
-                with open(env_path, "r") as f:
-                    for line in f:
-                        line = line.strip()
-                        if line and not line.startswith("#") and "=" in line:
-                            key, value = line.split("=", 1)
-                            existing_content[key.strip()] = value.strip()
-            except Exception as e:
-                _setup_log(f"SetupWizard: Could not read existing .env: {e}")
+            existing_content = dotenv_values(env_path) or {}

massgen/frontend/displays/textual_widgets/content_sections.py (3)

36-38: _env_flag duplicates tui_debug_enabled logic — consider reusing.

This helper replicates the same truthy-env-var check that tui_debug_enabled() in tui_debug.py uses. If more env flags are needed, this generic helper is fine. But if it's only used for MASSGEN_TUI_SCROLL_DEBUG (Line 445), you could document that relationship or import a shared helper to keep the "truthy env var" definition in one place.

Not blocking — just a minor DRY note.

---

813-843: Consider logging timer stop failures instead of silencing them.

The enter_final_lock method thoroughly resets all scroll state and cancels pending timers — good. However, the try/except pass blocks at Lines 824-828 and 829-834 silently swallow exceptions, which was flagged by Ruff (S110). Elsewhere in this file (Line 752-754), timer stop failures are logged via tui_log. For consistency:

♻️ Suggested fix

         if self._scroll_timer is not None:
             try:
                 self._scroll_timer.stop()
-            except Exception:
-                pass
+            except Exception as e:
+                tui_log(f"[ContentSections] enter_final_lock scroll_timer stop: {e}")
             self._scroll_timer = None
         if self._debounce_timer is not None:
             try:
                 self._debounce_timer.stop()
-            except Exception:
-                pass
+            except Exception as e:
+                tui_log(f"[ContentSections] enter_final_lock debounce_timer stop: {e}")
             self._debounce_timer = None

---

2591-2591: Back-compat alias _vote_results should stay in sync with vote_results.

self._vote_results = self.vote_results is set once in __init__. If vote_results is later mutated (e.g., dict updated in place), both references point to the same object, so they stay in sync. However, if self.vote_results is ever reassigned to a new dict, _vote_results will go stale. If external code only reads _vote_results, this is fine for the current usage, but a @property would be safer long-term.

massgen/frontend/displays/textual_widgets/file_explorer_panel.py (1)

142-143: Silent except Exception: pass hides scanning failures.

If os.walk or the loop body raises an unexpected error, the panel silently shows nothing with no way to diagnose why. Consider logging at debug level so issues are traceable without disrupting the TUI.

Proposed fix

+import logging
+
+logger = logging.getLogger(__name__)

-        except Exception:
-            pass
+        except Exception:
+            logger.debug("Error scanning workspace %s", self.workspace_path, exc_info=True)

pyproject.toml (1)

72-72: Consider making gitpython an optional dependency.

If the worktree/shadow-repo isolation features aren't used by all users, moving gitpython into an optional dependency group (e.g., [project.optional-dependencies] isolation = ["gitpython>=3.1.46"]) would keep the default install lighter. This is similar to how docker is already handled as an optional extra on line 110-112.

massgen/configs/debug/test_write_mode_isolation.yaml (1)

16-19: Prefer a cost‑effective model for example/debug configs.

Unless this test specifically needs Claude, consider switching to one of the preferred budget models (e.g., gpt‑5‑mini or gemini‑2.5‑flash).

As per coding guidelines massgen/configs/**/*.yaml: “Prefer cost-effective models (gpt-5-nano, gpt-5-mini, gemini-2.5-flash)”.

massgen/agent_config.py (1)

118-163: Add write_mode validation for programmatic configs.

CoordinationConfig can be instantiated directly (bypassing YAML validation). Consider validating write_mode in __post_init__ to fail fast on invalid values.

♻️ Suggested patch

@@
     def __post_init__(self):
         """Validate configuration after initialization."""
         self._validate_broadcast_config()
         self._validate_timeout_config()
+        self._validate_write_mode()
+
+    def _validate_write_mode(self) -> None:
+        """Validate write_mode configuration."""
+        if self.write_mode is None:
+            return
+        valid = {"auto", "worktree", "isolated", "legacy"}
+        if self.write_mode not in valid:
+            raise ValueError(
+                f"Invalid write_mode: {self.write_mode}. Must be one of {sorted(valid)} or None."
+            )

massgen/user_settings.py (2)

19-22: Annotate DEFAULT_SETTINGS with ClassVar.

Mutable class attributes should be annotated with typing.ClassVar to signal they aren't instance attributes. Flagged by Ruff (RUF012).

Proposed fix

+from typing import Any, ClassVar, Dict, Optional
-from typing import Any, Dict, Optional
 ...
-    DEFAULT_SETTINGS = {
+    DEFAULT_SETTINGS: ClassVar[Dict[str, Any]] = {
         "theme": "dark",
         "vim_mode": False,
     }

---

24-28: Missing docstring on __init__.

As per coding guidelines, new or changed functions should include Google-style docstrings. __init__ should document its behavior (loading settings from disk).

Proposed fix

 def __init__(self):
+    """Initialize UserSettings, loading from ~/.config/massgen/settings.json."""
     self._settings: Dict[str, Any] = {}

massgen/message_templates.py (1)

510-522: Docstring for branch_info understates the accepted type of other_branches.

Line 521 documents other_branches as list[str], but the implementation (Lines 556–560) also handles dict (label→branch mapping). The docstring should reflect both accepted formats to avoid confusion for callers.

Proposed docstring fix

-            branch_info: Optional dict with 'own_branch' (str) and 'other_branches' (list[str])
-                for communicating branch names from the previous attempt
+            branch_info: Optional dict with 'own_branch' (str) and 'other_branches'
+                (dict[str, str] mapping label to branch, or list[str] for legacy format)
+                for communicating branch names from the previous attempt

massgen/infrastructure/shadow_repo.py (1)

74-99: get_changes() duplicates git_utils.get_changes().

The logic here is identical to massgen/utils/git_utils.py:get_changes(repo). Consider delegating to avoid maintaining the same diff-enumeration code in two places.

Proposed fix

+from massgen.utils.git_utils import get_changes as git_get_changes
+
     def get_changes(self) -> List[Dict[str, str]]:
-        """
-        Get list of changes in the shadow repo since initial commit.
-
-        Returns:
-            List of dicts with 'status' and 'path' keys
-        """
+        """Get list of changes in the shadow repo since initial commit."""
         if not self.is_initialized or not self.repo:
             return []
-
-        changes = []
-
-        # Unstaged changes (working tree vs index)
-        for diff in self.repo.index.diff(None):
-            changes.append(
-                {
-                    "status": diff.change_type[0].upper(),  # M, A, D, R
-                    "path": diff.a_path or diff.b_path,
-                },
-            )
-
-        # Untracked files
-        for path in self.repo.untracked_files:
-            changes.append({"status": "?", "path": path})
-
-        return changes
+        return git_get_changes(self.repo)

massgen/frontend/coordination_ui.py (1)

824-829: Duplicated ensure_workspace_symlinks block across coordinate() and coordinate_with_context().

Both methods contain an identical 6-line block for symlink creation. This is consistent with the existing parallelism between these two methods, but worth noting as a candidate for extraction into a shared helper if these methods are ever refactored.

The broad Exception catch (flagged by Ruff BLE001) is acceptable here since symlink creation is non-critical and the orchestrator method already has its own internal exception handling (see massgen/orchestrator.py, Lines 528-548).

Also applies to: 1458-1463

docs/source/user_guide/agent_workspaces.rst (1)

82-83: Minor: RST underline length mismatch.

The title is 37 characters but the underline is 39. RST requires the underline to be at least as long as the title (so this works), but convention is to match exactly. Very minor nit.

docs/modules/worktrees.md (1)

9-23: Add language identifiers to fenced code blocks.

Three code blocks lack language specifiers (flagged by markdownlint MD040). Since these are ASCII diagrams and text blocks, text is appropriate:

Suggested fix

Line 9:

-```
+```text

Line 41:

-```
+```text

Line 59:

-```
+```text

Also applies to: 41-45, 59-65

massgen/infrastructure/__init__.py (1)

14-21: __all__ is not sorted (Ruff RUF022).

The filesystem_manager/__init__.py in this same PR uses alphabetical ordering. Align for consistency:

Suggested fix

 __all__ = [
+    "ShadowRepo",
     "WorktreeManager",
-    "ShadowRepo",
-    "get_git_root",
     "get_git_branch",
+    "get_git_root",
     "get_git_status",
     "is_git_repo",
 ]

massgen/filesystem_manager/_change_applier.py (2)

93-96: Chain the original exception for debuggability.

Ruff B904 is valid here — re-raising without from loses the original traceback context.

Proposed fix

         try:
             repo = Repo(str(source))
         except InvalidGitRepositoryError:
-            raise ValueError(f"Source is not a git repository: {source}")
+            raise ValueError(f"Source is not a git repository: {source}") from None

---

139-140: Use log.exception() instead of log.error() in exception handlers.

Within except blocks, log.exception() automatically includes the traceback, which is valuable for debugging file-apply failures. This applies to Lines 140, 189, and 240.

Proposed fix (example for line 140)

             except Exception as e:
-                log.error(f"Failed to apply change for {rel_path}: {e}")
+                log.exception(f"Failed to apply change for {rel_path}: {e}")

Also applies to: 188-189, 239-240

massgen/infrastructure/worktree_manager.py (2)

127-135: Silent error swallowing in _delete_branch loses diagnostic information.

If branch deletion fails (e.g., branch is checked out elsewhere), the bare pass makes it invisible. A debug-level log would aid troubleshooting without being noisy.

Proposed fix

         except GitCommandError:
-            pass
+            logger.debug(f"Could not delete branch {branch_name} (may already be deleted or in use)")

---

15-16: Missing Google-style class docstring.

The class has a one-liner docstring but no Attributes: section documenting repo and repo_path. As per coding guidelines, new classes in massgen/**/*.py should include Google-style docstrings.

massgen/frontend/displays/textual_themes/base.tcss (2)

4819-4857: Toast theming approach is sound; hardcoded hex duplicates the pattern from AgentStatusRibbon.

The override strategy (base → theme-class → severity) is correct and source-order ensures severity border-left wins over the theme default. Minor observation: the hardcoded hex values (#ffffff, #d0d7de, #0969da, etc.) for light/dark toasts duplicate the same GitHub-style palette values used in the AgentStatusRibbon and #input_header overrides. If these are already defined as variables in your palette files, referencing the variables would reduce the duplication and make future palette changes easier.

---

392-403: Hardcoded separator hex values duplicated across selectors.

The same hex pairs (#eaeef2 for light, #30363d for dark) appear in both AgentStatusRibbon (lines 398, 402) and #input_header (lines 1095, 1099). If $separator-border is already theme-aware in the palette, the .theme-light/.theme-dark overrides may be redundant. If not, consider extracting these into dedicated palette variables to avoid the duplication and make future color changes a single-point edit.

Also applies to: 1088-1100

massgen/frontend/displays/textual_terminal_display.py (1)

3861-3866: Don’t silently swallow preference persistence failures.

Failures to persist vim mode/theme are currently swallowed, which makes preference bugs very hard to diagnose. Consider logging at least a warning so users/devs can see why preferences didn’t save.

🛠️ Suggested fix (apply similarly to theme persistence)

             try:
                 user_settings = get_user_settings()
                 user_settings.vim_mode = self.question_input.vim_mode
-            except Exception:
-                pass
+            except Exception as e:
+                tui_log(f"[TextualDisplay] Failed to persist vim_mode: {e}")

Also applies to: 6261-6265

massgen/system_prompt_sections.py (1)

918-937: Update class docstring with the new constructor parameters.

The class-level docstring (lines 905–916) still lists only workspace_path, context_paths, and use_two_tier_workspace. The three new parameters—worktree_paths, branch_name, and other_branches—should be documented there as well to keep the Args block accurate.

As per coding guidelines, **/*.py: "For new or changed functions, include Google-style docstrings."

massgen/tests/test_write_mode_scratch.py (1)

381-402: Testing a private method (_apply_file_changes) directly.

This is acceptable for unit tests of internal logic, but note that if the method signature changes, this test will break without a public API change. Consider whether a thin public wrapper or integration test would be more maintainable long-term.

massgen/filesystem_manager/_isolation_context_manager.py (5)

265-267: Chain exceptions with raise ... from e to preserve traceback context.

Both _create_worktree_context and _create_shadow_context re-raise as RuntimeError but discard the original exception chain. Also, log.error won't include the traceback; prefer log.exception.

Proposed fix (showing worktree variant; apply same pattern to shadow)

         except Exception as e:
-            log.error(f"Failed to create worktree: {e}")
-            raise RuntimeError(f"Failed to create worktree context: {e}")
+            log.exception(f"Failed to create worktree: {e}")
+            raise RuntimeError(f"Failed to create worktree context: {e}") from e

         except Exception as e:
-            log.error(f"Failed to create shadow repo: {e}")
-            raise RuntimeError(f"Failed to create shadow context: {e}")
+            log.exception(f"Failed to create shadow repo: {e}")
+            raise RuntimeError(f"Failed to create shadow context: {e}") from e

Also applies to: 288-290

---

645-660: Silent try/except/pass blocks swallow errors during session cleanup.

These blocks (lines 648–652 and 657–660) silently discard exceptions, making it difficult to diagnose cleanup failures. Even at session end, a log.debug would aid troubleshooting stale branches or worktree metadata.

Proposed fix

         for branch_name in self._session_branches:
             for wm in self._worktree_managers.values():
                 try:
                     wm._delete_branch(branch_name, force=True)
                     log.info(f"Cleaned up session branch: {branch_name}")
-                except Exception:
-                    pass
+                except Exception as e:
+                    log.debug(f"Could not delete branch {branch_name}: {e}")
         self._session_branches.clear()
 
         # Prune any stale worktree metadata
         for wm in self._worktree_managers.values():
             try:
                 wm.prune()
-            except Exception:
-                pass
+            except Exception as e:
+                log.debug(f"Failed to prune worktree metadata: {e}")

---

744-761: get_diff for worktree mode stages and resets — non-atomic and leaks staged state on failure.

The add -A → diff --staged → reset HEAD sequence at lines 756–758 is a known trick, but if diff or reset throws (e.g., corrupted index), the worktree is left in a fully-staged state. Since this is an isolated worktree and the window is small, it's low risk, but worth noting.

Consider wrapping the reset in a finally block:

Proposed fix

                 # Stage everything so untracked (new) files appear in the diff,
                 # then unstage so ChangeApplier can still detect changes via
                 # repo.index.diff(None) and repo.untracked_files.
                 repo.git.add("-A")
-                diff_output = repo.git.diff("--staged")
-                repo.git.reset("HEAD")
-                return diff_output
+                try:
+                    diff_output = repo.git.diff("--staged")
+                finally:
+                    repo.git.reset("HEAD")
+                return diff_output

---

811-816: Another try/except/pass in cleanup_all — same silent swallowing concern as cleanup_session.

Same pattern as flagged in cleanup_session. Add log.debug for consistency.

Proposed fix

         for wm in self._worktree_managers.values():
             try:
                 wm.prune()
-            except Exception:
-                pass
+            except Exception as e:
+                log.debug(f"Failed to prune worktree metadata: {e}")

---

292-293: Method name _setup_scratch_in_worktree is misleading — it is also called for shadow repos.

Line 284 calls this from _create_shadow_context. Consider renaming to _setup_scratch_dir or _setup_scratch_in_isolation to reflect actual usage.

massgen/frontend/displays/textual/widgets/modals/content_modals.py (3)

390-403: Index-based button dispatch has a TOCTOU window if the path list changes between render and click.

on_button_pressed parses an integer index from the button ID and uses it to index into a fresh call to _get_current_paths() (inside _toggle_permission / _remove_path). If another agent or process modifies the paths between the render and the click, the index could refer to a different path—or be out of bounds (though the bounds check at lines 408/422 prevents a crash).

The risk is low in practice since changes require user action in the same modal, but worth noting.

---

289-301: _get_current_paths reads only from the first agent's PPM.

If agents' path lists ever diverge (e.g., a propagation partially fails), the modal will only reflect the first agent's state. Consider adding a defensive log if agents have inconsistent path sets, so divergence is surfaced early.

---

500-525: _update_agent_config_context_paths silently drops through when no keyword flags match.

If none of remove, add, or new_permission is truthy, the method exits without doing anything. This is technically correct since callers always set exactly one, but adding an else log or assertion would catch misuse.